Bugfix/issue 86 image upload assets not immediately available#156
Conversation
|
As discussed, I think a good UX would be to continously poll for changes on the manifest, and if a delta between the manifest content and our UI state is detected, then don't do an automatic refresh of the assets browser (which could be unnerving if a user is using the browser in that moment), but instead present an info bubble which says that the content on the image server has changed, and offer a call-to-action to manually refresh the browser. |
|
@manuelkiessling I've implemented a subtle info bubble which asks whether to re-render the image list or not. |
manuelkiessling
force-pushed
the
bugfix/issue-86-image-upload-assets-not-immediately-available
branch
from
f2cad9e to
45c3458
Compare
manuelkiessling
force-pushed
the
bugfix/issue-86-image-upload-assets-not-immediately-available
branch
from
45c3458 to
96b5c92
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Success message shown even when fetch fails
reloadAssetsAfterConfirmationnow returns early whenfetchAssets()returnsnull, so success UI is shown only after a successful refresh.
- ✅ Fixed: Stale DOM text used as fallback error label
- The fallback upload error label is now captured once from the initial DOM text on connect and reused, avoiding stale overwritten error messages.
- ✅ Fixed: Spurious refresh prompt when revision is initially null
checkForManifestUpdatesnow skips checks whileisLoadingis true, preventing refresh prompts during the initial manifest load race.
Or push these changes by commenting:
@cursor push 7aea7a69f6
Preview (7aea7a69f6)
diff --git a/src/RemoteContentAssets/Presentation/Resources/assets/controllers/remote_asset_browser_controller.ts b/src/RemoteContentAssets/Presentation/Resources/assets/controllers/remote_asset_browser_controller.ts
--- a/src/RemoteContentAssets/Presentation/Resources/assets/controllers/remote_asset_browser_controller.ts
+++ b/src/RemoteContentAssets/Presentation/Resources/assets/controllers/remote_asset_browser_controller.ts
@@ -104,6 +104,7 @@
private backgroundSyncTimeoutId: ReturnType<typeof setTimeout> | null = null;
private latestManifestRevision: string | null = null;
private isBackgroundSyncEnabled: boolean = false;
+ private uploadErrorFallbackLabel: string = "Upload failed. Please try again.";
private readonly focusHandler = (): void => {
void this.checkForManifestUpdates();
};
@@ -115,6 +116,13 @@
connect(): void {
this.isConnected = true;
+ if (this.hasUploadErrorTarget) {
+ const textEl = this.uploadErrorTarget.querySelector("[data-error-text]");
+ const initialText = textEl?.textContent?.trim();
+ if (initialText) {
+ this.uploadErrorFallbackLabel = initialText;
+ }
+ }
void this.fetchAssets();
this.setupDropzone();
this.isBackgroundSyncEnabled = this.getBackgroundSyncIntervalMs() > 0;
@@ -513,7 +521,7 @@
}
private async checkForManifestUpdates(): Promise<void> {
- if (this.isUploading) {
+ if (this.isUploading || this.isLoading) {
return;
}
@@ -560,7 +568,9 @@
}
private async reloadAssetsAfterConfirmation(): Promise<void> {
- await this.fetchAssets();
+ if ((await this.fetchAssets()) === null) {
+ return;
+ }
this.showUploadStatus("success");
setTimeout(() => this.showUploadStatus("none"), 3000);
}
@@ -572,13 +582,7 @@
}
private getUploadErrorFallbackLabel(): string {
- if (this.hasUploadErrorTarget) {
- const textEl = this.uploadErrorTarget.querySelector("[data-error-text]");
- if (textEl && textEl.textContent !== null && textEl.textContent !== "") {
- return textEl.textContent;
- }
- }
- return "Upload failed. Please try again.";
+ return this.uploadErrorFallbackLabel;
}
private normalizeManifestData(data: { urls?: string[]; revision?: string }): { urls: string[]; revision: string } {| await this.fetchAssets(); | ||
| this.showUploadStatus("success"); | ||
| setTimeout(() => this.showUploadStatus("none"), 3000); | ||
| } |
There was a problem hiding this comment.
Success message shown even when fetch fails
Medium Severity
reloadAssetsAfterConfirmation always shows the "success" status after calling fetchAssets(), regardless of whether the fetch succeeded or failed. Since fetchAssets returns null on failure, the return value can be checked — but it's currently ignored. If the network request fails when the user confirms a refresh, they'll see the empty state alongside a contradictory "Upload complete." success message.
| } | ||
| } | ||
| return "Upload failed. Please try again."; | ||
| } |
There was a problem hiding this comment.
Stale DOM text used as fallback error label
Low Severity
getUploadErrorFallbackLabel reads the current text content of the [data-error-text] element in the DOM. After any prior call to showUploadError, this element retains the previous error message (e.g., "1 of 3 uploads failed." or "still processing"). On a subsequent all-fail upload, this stale text is returned instead of the intended generic fallback, showing the user an incorrect or confusing error message.
| if (currentRevision === null || manifestData.revision !== currentRevision) { | ||
| this.latestManifestRevision = manifestData.revision; | ||
| this.markRefreshAvailable(); | ||
| } |
There was a problem hiding this comment.
Spurious refresh prompt when revision is initially null
Low Severity
checkForManifestUpdates treats currentRevision === null as a manifest change, triggering a refresh prompt. Since latestManifestRevision starts as null and is only set once fetchAssets completes, any focus or visibilitychange event that fires before that (e.g., user switches to a background-loaded tab) would cause a spurious "New images are available" prompt over content that was just loaded. The method guards against isUploading but not isLoading.
Additional Locations (1)
2 participants
Issue #86
When uploaded, assets are not immediately available and the upload status is unknown as long as it takes for uploading and processing.
Note
Medium Risk
Adds polling/background-sync and new API response fields, which changes client/server contract and introduces timing-driven behavior that could affect UX and load if misconfigured.
Overview
Fixes remote asset uploads not appearing immediately by tracking a manifest revision and waiting for uploaded files to show up in the manifest before prompting the user to refresh.
The
GET /api/projects/{projectId}/remote-assetsresponse now includesrevision(sha256 over sorted URLs), and the Stimulusremote-asset-browseradds manifest polling (including focus/visibility/background checks), a new processing/refresh prompt UI state with confirm/dismiss actions, and updated error/partial-failure messaging. Frontend unit tests anden/detranslations are updated to cover and label the new behaviors.Written by Cursor Bugbot for commit 6419900. This will update automatically on new commits. Configure here.